Skip to content

Fix reasoning_content error in tool calls for kimi-k2.5 and other thi…#876

Open
jdhxyy wants to merge 1 commit intosipeed:mainfrom
jdhxyy:main
Open

Fix reasoning_content error in tool calls for kimi-k2.5 and other thi…#876
jdhxyy wants to merge 1 commit intosipeed:mainfrom
jdhxyy:main

Conversation

@jdhxyy
Copy link

@jdhxyy jdhxyy commented Feb 27, 2026

…nking models

📝 Description

🗣️ Type of Change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 📖 Documentation update
  • ⚡ Code refactoring (no functional changes, no api changes)

🤖 AI Code Generation

  • 🤖 Fully AI-generated (100% AI, 0% Human)
  • 🛠️ Mostly AI-generated (AI draft, Human verified/modified)
  • 👨‍💻 Mostly Human-written (Human lead, AI assisted or none)

🔗 Related Issue

Fixes #588

📚 Technical Context (Skip for Docs)

  • Reference URL:
  • Reasoning:

🧪 Test Environment

  • Hardware:
  • OS:
  • Model/Provider:
  • Channels:

📸 Evidence (Optional)

Click to view Logs/Screenshots

☑️ Checklist

  • My code/docs follow the style of this project.
  • I have performed a self-review of my own changes.
  • I have updated the documentation accordingly.

Copy link

@nikolasdehor nikolasdehor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good targeted fix for #588. The field naming (ReasoningContent) is consistent with the existing Message struct. Two observations: (1) Incomplete fix? This PR preserves reasoning_content in the outbound serialization (stripSystemParts), but I don't see where response.ReasoningContent is copied into the assistant Message during tool loop execution. Without that, the ReasoningContent field will be empty for assistant messages created in toolloop.go. PR #889 addresses this in toolloop.go:109 — you may need to add that piece here too, or coordinate with that PR. (2) Unrelated .gitignore change: The __debug_bin patterns are fine but would be cleaner as a separate commit. (3) Test: Consider adding a test for stripSystemParts that verifies ReasoningContent is preserved. The core approach is correct — please verify the tool loop path is also covered.

@yinwm
Copy link
Collaborator

yinwm commented Feb 28, 2026

🔍 Deep Code Review

Thanks for the fix! The approach is correct, but I have some suggestions.


🔬 Problem Analysis

Root cause verified:

  1. protocoltypes.Message already has ReasoningContent field ✅
  2. parseResponse() correctly parses reasoning_content from API response ✅
  3. But stripSystemParts() was dropping ReasoningContent when serializing requests

This caused:

  • Turn 1: API returns reasoning_content, PicoClaw stores it correctly
  • Turn 2: When sending conversation history, reasoning_content is lost
  • Result: Moonshot API receives incomplete messages → 400 error

✅ Fix Assessment

The fix is correct - adding ReasoningContent to openaiMessage and passing it through in stripSystemParts().


⚠️ Missing Test Coverage

The existing test TestProviderChat_ParsesReasoningContent only tests response parsing, not request serialization.

We need a test to verify reasoning_content is preserved when sending conversation history:

func TestProviderChat_PreservesReasoningContentInHistory(t *testing.T) {
    var requestBody map[string]any

    server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        if err := json.NewDecoder(r.Body).Decode(&requestBody); err != nil {
            http.Error(w, err.Error(), http.StatusBadRequest)
            return
        }
        resp := map[string]any{
            "choices": []map[string]any{
                {"message": map[string]any{"content": "ok"}, "finish_reason": "stop"},
            },
        }
        w.Header().Set("Content-Type", "application/json")
        json.NewEncoder(w).Encode(resp)
    }))
    defer server.Close()

    p := NewProvider("key", server.URL, "")
    
    // Send conversation history with reasoning_content
    messages := []Message{
        {Role: "user", Content: "What is 1+1?"},
        {Role: "assistant", Content: "2", ReasoningContent: "Let me think... 1+1=2"},
        {Role: "user", Content: "What about 2+2?"},
    }
    
    _, err := p.Chat(t.Context(), messages, nil, "kimi-k2.5", nil)
    if err != nil {
        t.Fatalf("Chat() error = %v", err)
    }

    // Verify reasoning_content is preserved in the request
    reqMessages := requestBody["messages"].([]any)
    assistantMsg := reqMessages[1].(map[string]any)
    if assistantMsg["reasoning_content"] != "Let me think... 1+1=2" {
        t.Errorf("reasoning_content not preserved in request: got %v", assistantMsg["reasoning_content"])
    }
}

💡 Minor Suggestions

  1. .gitignore change: The __debug_bin* addition is unrelated to the main fix. Consider committing separately or noting in PR description.

  2. PR description: Consider filling in the "Technical Context" section with the reasoning about why this fix works.


📝 Summary

Type Count
🚨 Critical Issues 0
⚠️ Medium Issues 1 (missing request serialization test)
💡 Suggestions 2

Verdict: The fix is correct and can be merged after adding the test. Please add the test case above to ensure reasoning_content is properly preserved in multi-turn conversations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Moonshot kimi-k2.5 reasoning_content missing in tool calls leads to API Error 400

3 participants